Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sort by label keys before generating labels key and value lists #3698

Merged
merged 13 commits into from
Apr 17, 2024
Merged

Sort by label keys before generating labels key and value lists #3698

merged 13 commits into from
Apr 17, 2024

Conversation

soundofspace
Copy link
Contributor

@soundofspace soundofspace commented Feb 22, 2024 •
edited
Loading

Description

Fixes bug where labels are wrongfully assigned to values because their order changed in input dictionary.

Fixes part of #3391. This does not fix missing labels, it only fixes the issue of labels being wrongfully assigned even if all labels are present (also empty string label values).

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Test metric:

test_counter.add(1, {'cause': 'cause1', 'reason': 'reason1'})
test_counter.add(1, {'reason': 'reason2', 'cause': 'cause2'})

Before:

# HELP test_counter_total counter used for testing
# TYPE test_counter_total counter
test_counter_total{cause="reason1",reason="cause1"} 1.0
# HELP test_counter_total counter used for testing
# TYPE test_counter_total counter
test_counter_total{cause="cause2",reason="reason2"} 1.0

After:

# HELP test_counter_total counter used for testing
# TYPE test_counter_total counter
test_counter_total{cause="cause1",reason="reason1"} 1.0
test_counter_total{cause="cause2",reason="reason2"} 1.0

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated -> where?

@soundofspace soundofspace requested a review from a team February 22, 2024 10:14
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 22, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@ocelotl
Copy link
Contributor

ocelotl commented Mar 7, 2024

Can you please add a test case?

@ocelotl ocelotl added the bug Something isn't working label Mar 7, 2024
@ocelotl
Copy link
Contributor

ocelotl commented Mar 7, 2024

@lzchen

lzchen
lzchen requested changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog entry and a test for this.

@ocelotl
Copy link
Contributor

ocelotl commented Mar 14, 2024

@soundofspace this PR needs a test case, marking it as a draft until it has been added.

@ocelotl ocelotl marked this pull request as draft March 14, 2024 21:59
@soundofspace soundofspace marked this pull request as ready for review March 22, 2024 15:32
xrmx
xrmx approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's a trivial change and still an improvement compared to the actual code for some use cases LGTM

@soundofspace soundofspace requested a review from lzchen April 17, 2024 10:21
lzchen
lzchen approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please fix failing build.

@soundofspace
Copy link
Contributor Author

@lzchen not really sure why that build is failing because of this pr, seems like something unrelated to this, and not really sure how to fix it.

@ocelotl
Copy link
Contributor

ocelotl commented Apr 17, 2024

@lzchen not really sure why that build is failing because of this pr, seems like something unrelated to this, and not really sure how to fix it.

Probably we just need to update the contrib test SHA, no worries, I have done so.

@ocelotl ocelotl merged commit b51a6f8 into open-telemetry:main Apr 17, 2024
233 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xrmx xrmx xrmx approved these changes

@lzchen lzchen lzchen approved these changes

@ocelotl ocelotl ocelotl approved these changes

Assignees

@lzchen lzchen

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soundofspace @ocelotl @xrmx @lzchen